-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Remove Copilot completion request cancellation. #13012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Copilot completion request cancellation. #13012
Conversation
| return result; | ||
| } | ||
|
|
||
| // This is used to avoid processing unnecessary LSP cancel requests during a Copilot completion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sean-mcmanus the exact reason is that caching is happening on copilot-client, hence even if the token is cancelled, it means that the return value will NOT be used for the prompt construction, but it will be used for caching purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know it's not used for the currently running completion (it doesn't block Copilot), but it's cached and used on the next one. Does my comment not explain that? i.e. should I re-word my comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd omit the 2-minute detail since it is an implementation detail not located in this codebase. Instead, I'd mention that upon cancellation, the result will not be used to build the prompt (since the copilot extension gave up on waiting for a value from this provider), but the returned value is cached by the copilot extension and used on a subsequent call when available, where the cache will be hit and the call to getProjectContext will be saved.
The 2 minutes is the cache entry eviction time defined by the copilot extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I tried to re-word it.
Follow up to #12773 .
The previous/abandoned PR also ignored the ServerCancellation requests that occur if a user switches files and makes edits, but I added them back: #12988